RFC Host yielding API #2219

kaimast · 2021-03-29T22:01:05Z

Hi,

This is not intended to be merged into master yet, but I am curious about your thoughts on the approach. Essentially this pull request tries to fix/implement #1127 using the async-wormhole crate.

After enabling the async feature and you can call a function like so:

let instance = Instance::new(&module, import_object);

let stack = switcheroo::stack::EightMbStack;
instance.call_with_stack("my_func", stack);

This then executes the function in the userspace thread. You can yield host calls by exposing the Yielder to your WasmerEnv.

use wasmer::{LazyInit, Yielder};

#[ derive(WasmEnv) ]
sturct MyEnv {
     #[wasmer(yielder)]
      yielder: LazyInit<Yielder>
}

And then in the function call you can access the Yielder (which is just a clonable wrapper around async-wormhole's API).

fn my_host_func(env: &MyEnv) {
     let yielder = env.yielder.get_ref().unwrap().get();

     yielder.async_syspend(async {
             my_async_func().await;
     });
}

Hywan

I salute the amount of work to make all that working correctly! Thanks!

I'll need some times to review this PR carefully and to think about the feature design exactly :-). I reckon the feature is great, but it will take time to evaluate the design/the approach.

MarkMcCaskey

Interesting PR, thanks for going through and doing it. I don't understand how it all works yet, I'll probably have to look into what async-wormhole is doing to figure that out.

It's nice to have a proof of concept for this. I'm not currently well-informed enough to know what trade-offs this approach makes and if it's the right set of trade-offs for wasmer, but it seems minimally invasive...

Though changes like this in a system as complex as a Wasm VM can have a lot of side effects that we'll need to think through as well. I'll try to follow up on this more later once I've done some more research

MarkMcCaskey · 2021-03-30T15:06:28Z

lib/api/src/env.rs

+
+    #[ cfg(feature="async") ]
+    /// Set the thread-specific yielder
+    fn set_yielder(&mut self, _: *const std::ffi::c_void) {}


This probably needs to be unsafe and have a # Safety section describing how to correctly call and/or implement this function

MarkMcCaskey · 2021-03-30T15:09:13Z

lib/api/src/externals/function.rs

+        #[ cfg(feature="async") ]
+        let (host_env, metadata) =  {
+            let host_env_set_yielder_fn = |env_ptr, yielder_ptr| {
+                let env: &mut Env = unsafe { std::mem::transmute(env_ptr) };


It's nice to be super explicit about what exactly is being transmuted from what to what to make bugs and intent more obvious. I'd prefer all transmute being transmute::<A,B> with explicit types whenever possible.

MarkMcCaskey · 2021-03-30T15:11:23Z

lib/api/src/externals/function.rs

+#[ cfg(feature="async") ]
+fn build_export_function_metadata<Env>(
+    env: Env,
+    import_init_function_ptr: for<'a> fn(
+        &'a mut Env,
+        &'a crate::Instance,
+    ) -> Result<(), crate::HostEnvInitError>,
+    host_env_set_yielder_fn: fn(*mut std::ffi::c_void, *const std::ffi::c_void)
+) -> (*mut std::ffi::c_void, ExportFunctionMetadata)
+where
+    Env: Clone + Sized + 'static + Send + Sync,
+{
+    let import_init_function_ptr = Some(unsafe {
+        std::mem::transmute::<fn(_, _) -> Result<(), _>, fn(_, _) -> Result<(), _>>(
+            import_init_function_ptr,
+        )
+    });
+    let host_env_clone_fn: fn(*mut std::ffi::c_void) -> *mut std::ffi::c_void = |ptr| {
+        let env_ref: &Env = unsafe {
+            ptr.cast::<Env>()
+                .as_ref()
+                .expect("`ptr` to the environment is null when cloning it")
+        };
+        Box::into_raw(Box::new(env_ref.clone())) as _
+    };
+    let host_env_drop_fn: fn(*mut std::ffi::c_void) = |ptr| {
+        unsafe { Box::from_raw(ptr.cast::<Env>()) };
+    };
+    let env = Box::into_raw(Box::new(env)) as _;
+
+    // # Safety
+    // - All these functions work on all threads
+    // - The host env is `Send`.
+    let metadata = unsafe {
+        ExportFunctionMetadata::new(
+            env,
+            import_init_function_ptr,
+            host_env_clone_fn,
+            host_env_set_yielder_fn,
+            host_env_drop_fn,
+        )
+    };
+
+    (env, metadata)
+}
+
+#[ cfg(not(feature="async")) ]


I'd prefer having the cfg logic internal to the original function rather than having 2 copies just because it's so precise, dangerous, and error-prone. The reason this function exists in the first place is to try to avoid duplicating this kind of code.

MarkMcCaskey · 2021-03-30T15:12:38Z

lib/api/src/externals/function.rs

+        #[ cfg(feature="async") ]
+        let (host_env, metadata) = {
+            let host_env_set_yielder_fn = |env_ptr, yielder_ptr| {
+                let env: &mut Env = unsafe { std::mem::transmute(env_ptr) };


MarkMcCaskey · 2021-03-30T15:14:31Z

lib/api/src/externals/yielder.rs

+impl Yielder {
+    /// Create a new instance from a raw pointer to a yielder
+    pub fn new(inner: *const std::ffi::c_void) -> Self {
+        Self{ inner }
+    }
+
+    /// Get the `AsyncYielder`
+    pub fn get(&self) -> &mut AsyncYielder<Result<Box<[Val]>, RuntimeError>> {
+        let yielder: &mut AsyncYielder<Result<Box<[Val]>, RuntimeError>>= unsafe { std::mem::transmute(self.inner) };
+
+        yielder
+    }
+}


Either new needs to be unsafe or get needs to be unsafe (or possibly both)

MarkMcCaskey · 2021-03-30T15:16:18Z

lib/api/src/instance.rs

+                    let res = func.call_wasm(&wasm, params, &mut results);
+
+                    if let Err(e) = res {
+                        return Err(e);
+                    }


⛳

let res = func.call_wasm(&wasm, params, &mut results)?;

MarkMcCaskey · 2021-03-30T15:18:07Z

lib/api/src/module.rs

+            let instance_handle = self.artifact.instantiate(
+                self.store.tunables(),
+                resolver,
+                Box::new(()),
+            )?;


Looks like unrelated formatting changes, we use 1 behind latest stable (though I think we may not have updated to 1.50 in this repo yet), would be good to run that version of rustfmt on the code

MarkMcCaskey · 2021-03-30T15:20:38Z

lib/derive/src/parse.rs

+    #[ cfg(feature="async") ]
+    Yielder {
+        identifier: Option<LitStr>,
+        span: Span,
+    }


Minor nit (I haven't review all the derive macro changes yet) but we probably want to unconditionally parse this attribute and error if the feature isn't enabled or otherwise generate code that will provide a useful error message in that case. Don't worry about doing all that work now for this experimental PR, just writing this down for future reference.

kaimast · 2021-03-30T20:07:46Z

Thanks for taking a look. I should also note that the commit history is kind of a mess right now and the pull request makes some unnecessary changes, mainly because I pulled these changes out of a larger branch.

This reverts commit dad39c0.

syrusakbary · 2021-03-31T04:52:20Z

Some thoughts on the API design.

I believe a more ergonomic API would be:

Call async function

let instance = Instance::new(&module, import_object);

// It can be EightMbStack or OneMbStack
// This call is optional (and it can be EightMbStack by default if not called)
store.set_stack(EightMbStack);

instance.call_async("my_func");

Function definition

We can allow functions to be defined async, and detect them with the Rust type system (automatically wrapping them).

async fn my_host_func(env: &MyEnv) {
    my_async_func().await;
}

I'm not super convinced of having the yielder in the Env, but I'm not super sure what are the alternatives. Thoughts?

Apart from this, we will need to add tests and benchmark the calls (similarly as we do benchmarking of other function calls)

kaimast · 2021-04-30T23:06:08Z

Some thoughts on the API design.

I believe a more ergonomic API would be:

Call async function
let instance = Instance::new(&module, import_object);

// It can be EightMbStack or OneMbStack
// This call is optional (and it can be EightMbStack by default if not called)
store.set_stack(EightMbStack);

instance.call_async("my_func");
Function definition

We can allow functions to be defined async, and detect them with the Rust type system (automatically wrapping them).
async fn my_host_func(env: &MyEnv) {
    my_async_func().await;
}
I'm not super convinced of having the yielder in the Env, but I'm not super sure what are the alternatives. Thoughts?

Apart from this, we will need to add tests and benchmark the calls (similarly as we do benchmarking of other function calls)

I like these changes a lot. Will close this pull request for now and reopen once I have time to implement them.

Thanks for all the feedback!

Kai Mast added 13 commits December 26, 2020 21:23

Towards cloning VMs

2a0f7c9

Expose deep clone in API

638702d

More memcpy work

fe4e38f

Lint

666574c

Checkpoint

5d2203a

Merge remote-tracking branch 'vanilla/master' into clone

98278af

Test and compile fixes

8c0865f

Async API code

ae5ffa7

Experimental yielder API

f94beea

Port to newer async-womrhole and fix compilation

5df791e

Merge upstream master

708269e

Get rid of vm clone code

a8d8bbb

Undos some unneeded changes

fd30e36

kaimast requested review from Hywan, jubianchi, losfair, MarkMcCaskey, nlewycky and syrusakbary as code owners March 29, 2021 22:01

Hywan reviewed Mar 30, 2021

View reviewed changes

MarkMcCaskey reviewed Mar 30, 2021

View reviewed changes

Kai Mast added 5 commits March 30, 2021 22:41

Merge upstream master

4b04930

rustfmt

dad39c0

Revert "rustfmt"

ea8c891

This reverts commit dad39c0.

Use stable rustfmt

c14a97a

Undo unneeded changes

57ba569

Kai Mast added 2 commits April 21, 2021 23:32

Try fixing trap handling

729fad3

Try fixing trap handling

89b63d0

Kai Mast added 6 commits April 21, 2021 23:42

Try fixing trap handling

2cd5250

Track pre/post poll state

f70c791

Track pre/post poll state

01877eb

Remove assert

21730c3

Track pre/post poll state

99a6a0c

Track pre/post poll state

5ed888d

kaimast closed this Apr 30, 2021

kaimast mentioned this pull request May 3, 2021

Type parameter P in AsyncWormhole lunatic-solutions/async-wormhole#1

Open

kaimast mentioned this pull request Oct 23, 2023

Yielding from host calls #1127

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC Host yielding API #2219

RFC Host yielding API #2219

kaimast commented Mar 29, 2021

Hywan left a comment

MarkMcCaskey left a comment

MarkMcCaskey Mar 30, 2021

MarkMcCaskey Mar 30, 2021

MarkMcCaskey Mar 30, 2021

MarkMcCaskey Mar 30, 2021

MarkMcCaskey Mar 30, 2021

MarkMcCaskey Mar 30, 2021

MarkMcCaskey Mar 30, 2021

MarkMcCaskey Mar 30, 2021

kaimast commented Mar 30, 2021

syrusakbary commented Mar 31, 2021 •

edited

Loading

kaimast commented Apr 30, 2021

Call async function

Function definition

RFC Host yielding API #2219

RFC Host yielding API #2219

Conversation

kaimast commented Mar 29, 2021

Hywan left a comment

Choose a reason for hiding this comment

MarkMcCaskey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaimast commented Mar 30, 2021

syrusakbary commented Mar 31, 2021 • edited Loading

Call async function

Function definition

kaimast commented Apr 30, 2021

Call async function

Function definition

syrusakbary commented Mar 31, 2021 •

edited

Loading